Skip to content

Wait to free the holding cell during channel_reestablish handling #1859

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

TheBlueMatt
Copy link
Collaborator

When we process a channel_reestablish message we free the HTLC
update holding cell as things may have changed while we were
disconnected. However, some time ago, to handle freeing from the
holding cell when a monitor update completes, we added a holding
cell freeing check in get_and_clear_pending_msg_events. This
leaves the in-channel_reestablish holding cell clear redundant,
as doing it immediately or is get_and_clear_pending_msg_events is
not a user-visible difference.

Thus, we remove the redundant code here, substantially simplifying
handle_chan_restoration_locked while we're at it.

Asserting that specific log entries were printed isn't all that
useful, we should really be focusing on the expected messages (or,
when a monitor udpate fails, the lack thereof). In the next commit
one of these log checks would otherwise break due to the particular
time a monitor update fails changing, but I also plan on reworking
the montior update flows substantially soon, breaking lots of them.
When we process a `channel_reestablish` message we free the HTLC
update holding cell as things may have changed while we were
disconnected. However, some time ago, to handle freeing from the
holding cell when a monitor update completes, we added a holding
cell freeing check in `get_and_clear_pending_msg_events`. This
leaves the in-`channel_reestablish` holding cell clear redundant,
as doing it immediately or is `get_and_clear_pending_msg_events` is
not a user-visible difference.

Thus, we remove the redundant code here, substantially simplifying
`handle_chan_restoration_locked` while we're at it.
There is no reason anymore for `handle_chan_restoration_locked` to
be a macro, and our long-term desire is to move away from macros as
they substantially bloat our compilation time (and binary size).
Thus, we simply remove `handle_chan_restoration_locked` here and
turn it into a function.
@TheBlueMatt TheBlueMatt force-pushed the 2022-11-rm-redundant-holding-cell-wipe branch from 755f15c to f1c6cd8 Compare November 17, 2022 17:57
@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2022

Codecov Report

Base: 90.61% // Head: 90.70% // Increases project coverage by +0.09% 🎉

Coverage data is based on head (f1c6cd8) compared to base (7269fa2).
Patch coverage: 81.66% of modified lines in pull request are covered.

❗ Current head f1c6cd8 differs from pull request most recent head e82cfa7. Consider uploading reports for the commit e82cfa7 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1859      +/-   ##
==========================================
+ Coverage   90.61%   90.70%   +0.09%     
==========================================
  Files          90       90              
  Lines       47623    47552      -71     
  Branches    47623    47552      -71     
==========================================
- Hits        43152    43132      -20     
+ Misses       4471     4420      -51     
Impacted Files Coverage Δ
lightning/src/ln/functional_test_utils.rs 93.64% <ø> (ø)
lightning/src/ln/channelmanager.rs 86.19% <76.74%> (+1.15%) ⬆️
lightning/src/ln/channel.rs 88.83% <88.88%> (+0.08%) ⬆️
lightning/src/ln/chanmon_update_fail_tests.rs 97.58% <100.00%> (-0.03%) ⬇️
lightning/src/ln/reload_tests.rs 95.24% <100.00%> (ø)
lightning/src/ln/functional_tests.rs 97.06% <0.00%> (-0.10%) ⬇️
lightning/src/chain/onchaintx.rs 94.23% <0.00%> (+0.21%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM I think

Now that `handle_channel_resumption` can't fail, the error handling
in `post_handle_chan_restoration` is now dead code. Removing it
makes `post_handle_chan_restoration` only a single block, so here
we simply remove the macro and inline the single block into the two
places the macro was used.
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Between this Screen Shot 2022-11-21 at 2 32 07 PM and getting rid of a confusing macro, candidate for PR of the year

@TheBlueMatt TheBlueMatt merged commit 8245128 into lightningdevkit:main Nov 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants